Configure app from file using internal config package #35

sacha-c · 2024-12-16T12:52:06Z

Description
The TOML parsing offered by the CLI package we use is limited and doesn't seem to support dictionaries, etc (see here)

To be able to fully use TOML and therefore provide the nicest interface we can, I propose here to parse the config ourselves.

To do this, I first moved some files around in #34
And here I am using this new internal config package to parse the config file ourselves.

Changes made
As a recap:

Removed Go CLI config parsing in favor of our internal config.GetPatrolConfiguration
- This receives the CLI options and merges the CLI options with the parsed file configuration
- Order of priority remains the same, envvars > cli opts > file
Refactored project & app file configurations so they use the same toml parsing method in our internal toml package
Allowed me to remove some complexity (CombineBeforeFuncs out! GetConfigFileLoader out! Go CLI parsing out!)

Interface discussion
There are some interface changes. Not sure if it's worth it to show the comparison, maybe I can just post the new interface and we can discuss from there.
CLI options (unchanged in this PR)

OPTIONS:
   --config value, -c value  (default: "sheriff.toml")

   Miscellaneous:

   --verbose, -v  Enable verbose logging (default: false)

   Reporting (configurable by file):

   --report-enable-project-report-to                    Enable project-level configuration for '--report-to-*'. (default: true)
   --report-to-email value [ --report-to-email value ]  Enable reporting to the provided list of emails
   --report-to-issue                                    Enable or disable reporting to the project's issue on the associated platform (gitlab, github, ...) (default: false)
   --report-to-slack-channel value                      Enable reporting to the provided slack channel
   --silent                                             Disable report output to stdout. (default: false)

   Scanning (configurable by file):

   --url value [ --url value ]  Groups and projects to scan for vulnerabilities (list argument which can be repeated)

   Tokens:

   --gitlab-token value  Token to access the Gitlab API. [$GITLAB_TOKEN]
   --slack-token value   Token to access the Slack API. [$SLACK_TOKEN]

File configuration

urls = ["gitlab://group1/project2"]

[report]
silent = true

[report.to]
emails = ["[email protected]"]
issue = true
slack-channel = "sheriff-reports"
enable-project-report-to = false

scastlara · 2024-12-16T13:46:54Z

For parity's sake, shouldn't emails and slack-channel be either both slices, or both a single value?
(not for this PR, but since this is also a discussion of what we want the interface to be...)

It is strange that they are different, because fundamentally they kinda are the same, no?

sacha-c · 2024-12-16T13:49:53Z

For parity's sake, shouldn't emails and slack-channel be either both slices, or both a single value? (not for this PR, but since this is also a discussion of what we want the interface to be...)

It is strange that they are different, because fundamentally they kinda are the same, no?

Yes this is absolutely to be discussed in this PR. I was thinking of making the change, but decided against it before we addressed it.

I'm totally find having the slack channels be a list. However I do fail to see in which scenario would it be used as a list 🤔 Whereas for emails it is pretty clear. So it makes me wonder if we should make it a list for parity's sake, or keep it a value for how it will be used

scastlara

Looks great!

Just a comment about docstrings not following conventions, and a question about the API parity between slack/email!

internal/config/patrol.go

scastlara · 2024-12-16T13:44:51Z

internal/config/patrol.go

 }

+// Returns valueA if != nil, otherwise valueB if != nil, otherwise the provided default value
+func getCliOrFileOption[T interface{}](valueA *T, valueB *T, def T) (r T) {


internal/toml/parse.go

scastlara · 2024-12-16T13:53:07Z

For parity's sake, shouldn't emails and slack-channel be either both slices, or both a single value? (not for this PR, but since this is also a discussion of what we want the interface to be...)
It is strange that they are different, because fundamentally they kinda are the same, no?

Yes this is absolutely to be discussed in this PR. I was thinking of making the change, but decided against it before we addressed it.

I'm totally find having the slack channels be a list. However I do fail to see in which scenario would it be used as a list 🤔 Whereas for emails it is pretty clear. So it makes me wonder if we should make it a list for parity's sake, or keep it a value for how it will be used

I could see some companies/groups/teams being organized in a way such that they want to send the reports to multiple slack channels: one for security, one for PMs, one for Devs. Or any other combination. Not saying it is the most useful thing to do, but it is not a crazy feature, especially since the API will kinda be prepared to send a report to multiple places anyway (because of emails).

scastlara · 2024-12-16T14:00:21Z

Thinking about the API "holistically" (so not this PR, just before 1.0.0) I am still not sold on url as an arg name. It kinda makes people believe they can just slap their repo url. Something like target I would like more.

Changes urls -> targets Changes slack-channel -> slack-channels Adds readme with details of all configuration options

sacha-c changed the base branch from main to config-toml-refactor December 16, 2024 12:56

sacha-c force-pushed the config-toml branch 2 times, most recently from dda6956 to 5f63567 Compare December 16, 2024 13:01

scastlara requested changes Dec 16, 2024

View reviewed changes

scastlara approved these changes Dec 18, 2024

View reviewed changes

sacha-c force-pushed the config-toml branch from 6a4bdf9 to 67d74ca Compare December 18, 2024 09:10

sacha-c changed the base branch from config-toml-refactor to main December 18, 2024 09:12

sacha-c added 3 commits December 18, 2024 10:13

feat(#32): configure by file with internal package

b7d9c0e

feat(#32): improve configuration API

a53f9d5

Changes urls -> targets Changes slack-channel -> slack-channels Adds readme with details of all configuration options

chore: move toml function to config package and improve comments

de9ea78

sacha-c force-pushed the config-toml branch from 67d74ca to de9ea78 Compare December 18, 2024 09:13

sacha-c merged commit a1e2551 into main Dec 18, 2024
3 checks passed

sacha-c deleted the config-toml branch December 23, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Configure app from file using internal config package #35

Configure app from file using internal config package #35

Uh oh!

sacha-c commented Dec 16, 2024

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

sacha-c commented Dec 16, 2024

Uh oh!

scastlara left a comment

Uh oh!

Uh oh!

scastlara Dec 16, 2024

Uh oh!

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Configure app from file using internal config package #35

Configure app from file using internal config package #35

Uh oh!

Conversation

sacha-c commented Dec 16, 2024

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

sacha-c commented Dec 16, 2024

Uh oh!

scastlara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scastlara Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

scastlara commented Dec 16, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants